Skip to content

Resolve chipmunk execution issues in Ubuntu 24#2478

Open
itsmesamster wants to merge 1 commit intoesrlabs:masterfrom
itsmesamster:master
Open

Resolve chipmunk execution issues in Ubuntu 24#2478
itsmesamster wants to merge 1 commit intoesrlabs:masterfrom
itsmesamster:master

Conversation

@itsmesamster
Copy link
Collaborator

No description provided.

Copy link
Member

@AmmarAbouZor AmmarAbouZor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great 👍

I added a comment regarding moving the whole sandbox checks into one function that will run on Linux only.

Also, we need to increase the version of the development cli tool with this change.

Comment on lines +305 to +312
let sandbox = Target::App.cwd().join("node_modules/electron/dist/chrome-sandbox");
if sandbox.exists() {
if let Err(e) = fix_electron_sandbox(sandbox).await {
eprintln!("Warning: Failed to fix Electron sandbox permissions: {}", e);
eprintln!("You may need to run manually:");
eprintln!(" sudo chown root <path> && sudo chmod 4755 <path>");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would include the whole check logic inside fix_electron_sandbox calling it ensure_electron_sandbox since the sandbox check is related to Linux only

@AmmarAbouZor
Copy link
Member

Also the CI is failing because of a clippy warning in development cli tool

Copy link
Member

@AmmarAbouZor AmmarAbouZor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working great.
I'm just wondering for the case if end-users installed chipmunk from the archives and not via .deb installer. Will Chipmunk works for them form the command line?
I think we still need to provide a script that they can run once with sudo to solve the problem for them. Then we can extend our README with a command to download the script (via curt) and run it

Copy link
Member

@AmmarAbouZor AmmarAbouZor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in App armor looks solid. We need to apply them for the development cli tool as well replacing the chrom-sandbox permission fix with it

Comment on lines +328 to +363
let sandbox = Target::App
.cwd()
.join("node_modules/electron/dist/chrome-sandbox");

if sandbox.exists() {
let meta = std::fs::metadata(&sandbox)?;
if meta.uid() == 0 && (meta.permissions().mode() & 0o4000) != 0 {
return Ok(());
}

eprintln!("\n[!] ACTION REQUIRED: Fixing Electron Sandbox permissions.");
eprintln!("[!] Please enter your password for sudo:");

let status = std::process::Command::new("sudo")
.arg("sh")
.arg("-c")
.arg(format!(
"chown root {0} && chmod 4755 {0}",
sandbox.display()
))
.stdin(std::process::Stdio::inherit())
.stdout(std::process::Stdio::inherit())
.stderr(std::process::Stdio::inherit())
.status()?;

if status.success() {
eprintln!("[+] Permissions fixed successfully.\n");
Ok(())
} else {
eprintln!("You may need to run manually:");
eprintln!(" sudo chown root <path> && sudo chmod 4755 <path>");
anyhow::bail!("Sudo prompt failed or was cancelled by user.")
}
} else {
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that we will skip the permission for chrome-sandbox here and will check for apparmor here as well.

Is it possible to check for app-armor here and verify that Chipmunk is enabled? If that possible I would check for that then run the script to ensure we have matching environment between development and end-users on Ubuntu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants